-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
added UpdatePc tests #61
Conversation
I think they are the same, because when jzn -> res = op1 |
Offsets can be negative, but it is express according to Field Elements for example, -1 would be:
Because operations with felt are made modulus a prime P where P = x + 1. Then x + 3 = 2, for example |
Don't worry about negative offset nonetheless, I am fixing them as part of a feature I am working on |
Do you mean in the update res part of the pseudo code res = op1? If so, isn't that when pc_update is case 0,1 or 2? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good! Just a nitpick and it ready to merge
You were right from the beginning, it was a bug, good catch! I think it should be already patched in |
* added UpdatePc tests and fixed a bug in UpdatePc * Fixed accessing field value without Read() * fixed failing tests * small refactor for TestUpdatePcJump
* edited comments todo list * verify Compute Result Addition via more test results. Minor update on add function on memory_value.go * Verify Test for ComputeRes Addition * improve better format for test function * separate each cases to have own functions * add Mulitple operand test cases and also Unconstrained, Op1 * wip memory addition * Fixes add function with felt-to-address addition * added UpdatePc tests (#61) * added UpdatePc tests and fixed a bug in UpdatePc * Fixed accessing field value without Read() * fixed failing tests * small refactor for TestUpdatePcJump * Chore: Readme hotfix * separate each cases to have own functions * Fix as per comments of prev. PR. Fixes issue #56 adding test cases and code change * integration test done * minor change on comments * Remove factorial_compiled.json * Fix add implementation --------- Co-authored-by: M. Mahdi Khosravi <mmk1776@gmail.com> Co-authored-by: Rodrigo <rodrodpino@gmail.com>
I have added 5 tests for UpdatePc function. I could not add tests with negative offsets because the offset should be uint64. In case there is any way to address this, please point it out and I'll add negative offset test variants too.
Also while I was writing tests, I noticed a potential bug. Based on the Cairo whitepaper, in the case of JNZ (conditional relative jump), the next_pc should be
pc + op1
notpc + res
, so I fixed that too. If I understood incorrectly please lmk.